centralize b-value management at initialization (#155)#165
centralize b-value management at initialization (#155)#165Devguru-codes wants to merge 1 commit into
Conversation
- Remove bvalues parameter from all 27 algorithm ivim_fit() and ivim_fit_full_volume() signatures in src/standardized/ - All algorithms now access b-values via self.bvalues exclusively - Add **kwargs to algorithms that lacked it (OJ_GU_seg, PV_MUMC, PvH_KB_NKI, TF_reference) - OsipiBase.osipi_fit() and osipi_fit_full_volume() enforce ValueError if bvalues not set at init - Update all test files to pass bvalues at OsipiBase(...) init - Update WrapImage production wrappers to use new API - Remove now-redundant bvalue equality checks in DL algorithms Files modified: 33 (27 algorithms + OsipiBase + 3 tests + 2 wrappers)
|
Hello @oliverchampion sir, please review the changes. If anything needs to be updated, let me know. Thank you. |
| bvalues = self.bvalues | ||
| else: | ||
| bvalues = np.asarray(bvalues) | ||
| bvalues = np.asarray(self.bvalues) |
There was a problem hiding this comment.
Given the if statement above, we probably do not need the np.asarray. self.bvalues has alrady been set in an arrray.
There was a problem hiding this comment.
(for all IAR's algorithms)
| initial_guess = [self.initial_guess["D"], self.initial_guess["f"], self.initial_guess["Dp"], self.initial_guess["S0"]] | ||
|
|
||
| bvalues=np.array(bvalues) | ||
| bvalues=np.array(self.bvalues) |
There was a problem hiding this comment.
For all algorithms: do we need to set it als bvalues=np.array(self.bvalues)? Or could we not just use self.bvalues instead of bvalues?
| "b-values must be provided at initialization (__init__), not at fit-time. " | ||
| "Pass bvalues to the constructor: OsipiBase(bvalues=..., algorithm=...)" | ||
| ) | ||
| use_bvalues = np.asarray(self.bvalues) |
There was a problem hiding this comment.
I think this means we can skip all other np.array/np.asarray
|
Otherwise it looks good to me. I'm a little worried that stuf down the line may break, such as the longitudinal testing and the website deployment; these are only run once merged. But let's cross that bridge when we get there. Maybe @IvanARashid can check the osipi_base. |
2 participants
fixes #155